Add characteristic discovery to BleService #2203

mgolu · 2020-09-23T13:21:09Z

This PR is targeting to the feature/ble_cpp_callback/ch65803 branch

Problem

Allows for hierarchical discovery of characteristics inside services.

Solution

Adds a discoverAllCharacteristics function to BleServiceImpl class.

Steps to Test

peer->discoverAllServices();
peer.getServiceByUUID(service, );
service.discoverAllCharacteristics();

Completeness

User is totes amazing for contributing!
Contributor has signed CLA (Info here)
Problem and Solution clearly stated
Run unit/integration/application tests on device
Added documentation
Added to CHANGELOG.md after merging (add links to docs and issues)

wiring/src/spark_wiring_ble.cpp

wiring/inc/spark_wiring_ble.h

XuGuohui

Thank you @mgolu for submitting this great PR! There is still something we can improve as I commented. If you'd like, I'm glad to take over this PR to make the final change.

mgolu · 2020-09-23T14:21:29Z

Thank you @mgolu for submitting this great PR! There is still something we can improve as I commented. If you'd like, I'm glad to take over this PR to make the final change.

Yes, great comments. I agree we can refactor, I just didn't want to touch existing APIs yet. If you have the time to work on it, go for it!

wiring/src/spark_wiring_ble.cpp

mgolu

This is great to save RAM by keeping just one copy of characteristics. I think the application will have to keep track of the peer associated with each service (for example, when connecting to multiple peers).

Maybe we can add a call to the BleService class that calls the peer's discoverCharacteristicsOfService? That way the application doesn't have to keep track of which peer is for each service.

wiring/src/spark_wiring_ble.cpp

XuGuohui · 2020-09-30T05:57:06Z

Maybe we can add a call to the BleService class that calls the peer's discoverCharacteristicsOfService?

The initial objective of the BLE wiring APIs are designed to be easy to use for non professional developers. Thus, we just provide two main mechanisms for BLE operation, data transaction oriented and non-data transaction oriented. For non-data transaction oriented operation, we have the BLE object for local device operation and the BlePeerDevice for peer device operation. And for data transaction oriented class, we have the BleCharacteristic class. The BleService class doesn't need to touch anything about data/non-data transaction operation. I'd prefer not to introduce more APIs under the service class to make things complicate. Could you elaborate how it affects if the central is connected with multiple peers while the BleService doesn't associated with a certain peer?

mgolu · 2020-09-30T13:37:00Z

The initial objective of the BLE wiring APIs are designed to be easy to use for non professional developers. Thus, we just provide two main mechanisms for BLE operation, data transaction oriented and non-data transaction oriented. For non-data transaction oriented operation, we have the BLE object for local device operation and the BlePeerDevice for peer device operation. And for data transaction oriented class, we have the BleCharacteristic class. The BleService class doesn't need to touch anything about data/non-data transaction operation. I'd prefer not to introduce more APIs under the service class to make things complicate. Could you elaborate how it affects if the central is connected with multiple peers while the BleService doesn't associated with a certain peer?

The use case would be for an Enterprise customer who might have multiple peers connected, which might be the same Profile or type. Then, they would have multiple BleService instances with the same UUID, but related to different peers. If they want to discover characteristics on an instance of BleService, that's currently impossible since there's no API on BleService to discover characteristics, and there's no reference to the BlePeerDevice that the instance is associated with. That means that the developer needs to keep track themselves of each BleService instance association to a BlePeer.

XuGuohui · 2020-09-30T16:29:30Z

I see no reason to keep tracking a BLE service with a peer device in user application. For discovering characteristics under a peer associated service, we can call peer.discoverCharacteristicsOfService() that is introduced in this PR. Other than that, we don't need to operate a BLE service. Whenever we want to retrieve a peer associated service, you can call peer.services() or peer.getServiceByUUID(). So we just need to track the peer devices it connected. As for data transmission, we use characteristic methods, which is already peer associated.

mgolu · 2020-10-08T13:32:06Z

I tested with the new proposed BLE Gateway library (https://github.com/particle-iot/ble-gateway-library) and the new changes are working well. Just submitted a small fix.

XuGuohui · 2020-10-14T06:50:22Z

@avtolstoy Do you have any comment here?

mgolu · 2020-11-10T16:47:56Z

Can this branch be merged into develop?

XuGuohui · 2020-11-11T05:46:43Z

Can this branch be merged into develop?

We will get it merged in this sprint (in two weeks). Cc. @avtolstoy

avtolstoy · 2020-11-18T19:54:31Z

wiring/src/spark_wiring_ble.cpp

@@ -1001,10 +1002,24 @@ class BleServiceImpl {
        return endHandle_;
    }

+    bool& characteristicsDiscovered() {


#1847 (comment) :)

Nit: This is a really weird pattern when considering trivial types. I'd rather have this as a public member then or use setters/getters.

existing char instead of passed value

mgolu added enhancement needs review labels Sep 23, 2020

mgolu requested review from avtolstoy and XuGuohui September 23, 2020 13:21

XuGuohui reviewed Sep 23, 2020

View reviewed changes

wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved

XuGuohui reviewed Sep 23, 2020

View reviewed changes

wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved

XuGuohui reviewed Sep 23, 2020

View reviewed changes

wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved

XuGuohui reviewed Sep 23, 2020

View reviewed changes

wiring/inc/spark_wiring_ble.h Outdated Show resolved Hide resolved

XuGuohui requested changes Sep 23, 2020

View reviewed changes

XuGuohui self-assigned this Sep 23, 2020

XuGuohui added this to the 3.0.0 milestone Sep 23, 2020

XuGuohui force-pushed the ch63765/add_ble_char_discovery_to_service branch from 502e15d to 581f75a Compare September 28, 2020 13:45

XuGuohui added the ble Bluetooth Low Energy label Sep 28, 2020

mgolu commented Sep 28, 2020

View reviewed changes

wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved

mgolu commented Sep 28, 2020

View reviewed changes

wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved

wiring/src/spark_wiring_ble.cpp Outdated Show resolved Hide resolved

XuGuohui approved these changes Oct 14, 2020

View reviewed changes

XuGuohui force-pushed the ch63765/add_ble_char_discovery_to_service branch from 7a255ce to 7f1806e Compare October 26, 2020 13:53

XuGuohui changed the base branch from develop to feature/ble_scan_filter/ch37224 October 26, 2020 13:54

XuGuohui changed the base branch from feature/ble_scan_filter/ch37224 to feature/ble_cpp_callback/ch65803 October 26, 2020 13:55

avtolstoy approved these changes Nov 18, 2020

View reviewed changes

XuGuohui force-pushed the feature/ble_cpp_callback/ch65803 branch from d567d5b to e3967bb Compare November 20, 2020 13:28

Base automatically changed from feature/ble_cpp_callback/ch65803 to develop November 20, 2020 14:12

XuGuohui force-pushed the ch63765/add_ble_char_discovery_to_service branch from 0abf96c to 84ebbd9 Compare November 20, 2020 15:41

mgolu and others added 7 commits November 20, 2020 23:53

Add characteristic discovery to BleService

70ba091

[wiring] BLE: add API to discover characteristics under a service.

4dee1a3

[wiring] BLE: add more apis.

55bab0b

Fix inability to discover: check handles of

d6b45f3

existing char instead of passed value

[wiring] BLE: add api test.

2b20dea

[wiring] BLE: minor

ccaff13

[wiring] BLE: minor

28175ba

XuGuohui force-pushed the ch63765/add_ble_char_discovery_to_service branch from 84ebbd9 to 28175ba Compare November 20, 2020 16:08

XuGuohui requested a review from avtolstoy November 20, 2020 16:09

avtolstoy approved these changes Nov 20, 2020

View reviewed changes

XuGuohui merged commit 9d89600 into develop Nov 20, 2020

XuGuohui deleted the ch63765/add_ble_char_discovery_to_service branch November 20, 2020 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add characteristic discovery to BleService #2203

Add characteristic discovery to BleService #2203

mgolu commented Sep 23, 2020 •

edited by XuGuohui

Loading

XuGuohui left a comment

mgolu commented Sep 23, 2020

mgolu left a comment

XuGuohui commented Sep 30, 2020

mgolu commented Sep 30, 2020

XuGuohui commented Sep 30, 2020

mgolu commented Oct 8, 2020

XuGuohui commented Oct 14, 2020

mgolu commented Nov 10, 2020

XuGuohui commented Nov 11, 2020

avtolstoy Nov 18, 2020

Add characteristic discovery to BleService #2203

Add characteristic discovery to BleService #2203

Conversation

mgolu commented Sep 23, 2020 • edited by XuGuohui Loading

Problem

Solution

Steps to Test

Completeness

XuGuohui left a comment

Choose a reason for hiding this comment

mgolu commented Sep 23, 2020

mgolu left a comment

Choose a reason for hiding this comment

XuGuohui commented Sep 30, 2020

mgolu commented Sep 30, 2020

XuGuohui commented Sep 30, 2020

mgolu commented Oct 8, 2020

XuGuohui commented Oct 14, 2020

mgolu commented Nov 10, 2020

XuGuohui commented Nov 11, 2020

avtolstoy Nov 18, 2020

Choose a reason for hiding this comment

mgolu commented Sep 23, 2020 •

edited by XuGuohui

Loading